Move git config/remote to gitrepo package and add global lock to resolve possible conflict when updating repository git config file #35151

lunny · 2025-07-24T05:01:02Z

Partially fix #32018

git config and git remote write operations create a temporary file named config.lock. Since these operations are not atomic, they must not be run in parallel. If two requests attempt to modify the same repository concurrently—such as during a compare operation—one may fail due to the presence of an existing config.lock file.

In cases where config.lock is left behind due to an unexpected program exit, a global lock mechanism could allow us to safely remove the stale lock file when a related error is detected. While this behavior is not yet implemented in this PR, it is planned for a future enhancement.

…lve possible conflict when updating repository git config file

wxiaoguang · 2025-07-24T12:41:02Z

No, it doesn't fix #32018

wxiaoguang

Don't see why it is worth or right to do so, and don't see why it fixes that issue

lunny · 2025-07-24T17:22:24Z

Don't see why it is worth or right to do so, and don't see why it fixes that issue

git config and git remote write operations create a temporary file named config.lock. Since these operations are not atomic, they must not be run in parallel. If two requests attempt to modify the same repository concurrently—such as during a compare operation—one may fail due to the presence of an existing config.lock file.

In cases where config.lock is left behind due to an unexpected program exit, a global lock mechanism could allow us to safely remove the stale lock file when a related error is detected. While this behavior is not yet implemented in this PR, it is planned for a future enhancement.

wxiaoguang · 2025-07-25T01:30:25Z

So it is an improvement but doesn't "Fix #32018", right?

But does it really need the "global lock"? Is it possible that git already uses "*.lock" files by flock for global lock? Then Gitea shouldn't do anything more?

lunny · 2025-07-25T02:10:28Z

So it is an improvement but doesn't "Fix #32018", right?

But does it really need the "global lock"? Is it possible that git already uses "*.lock" files by flock for global lock? Then Gitea shouldn't do anything more?

The Git command-line interface is primarily designed for human interaction. When a git config write operation is initiated and a config.lock file is created, any subsequent git config write operation will immediately fail instead of waiting for the first one to complete.

While I couldn’t find official documentation confirming this behavior, the implementation can be found in the Git source code here:
https://github.com/git/git/blob/97e14d99f6def189b0f786ac6207b792ca3197b1/config.c#L3199

Based on this, it appears that Git does not implement a locking mechanism that causes subsequent operations to wait—it simply fails fast.

I believe this change partially addresses #32018, as there are three possible causes for the issue:

Concurrent requests attempting to compare between the same base and forked repositories.
A Git command exits unexpectedly, leaving behind a config.lock file. I plan to implement automatic cleanup of stale config.lock files in another PR, which would help resolve this case.
An administrator manually runs Git commands on the Gitea server. This scenario is outside the scope of this PR, and I don’t believe it’s something we can reliably handle within the application.

wxiaoguang · 2025-07-25T02:21:03Z

Git already does a file-based "global lock"

~~https://github.com/git/git/blob/master/lockfile.c#L104 (config.c hold_lock_file_for_update -> lockfile.c lock_file_timeout )~~

~~I do not think this PR is worth or right.~~

Update: git passes timeout=0, so it only locks "once"

dismiss

modules/gitrepo/config.go

…a into lunny/fix_git_config_conflict

lunny · 2025-08-19T17:18:55Z

last call @go-gitea/technical-oversight-committee

wxiaoguang · 2025-08-19T19:08:52Z

modules/gitrepo/config.go

+// No lock is needed because the remote remoteName will be checked before invoking this function.
+// Then it will not update the remote automatically if the remote does not exist.
+func GitRemoteUpdatePrune(ctx context.Context, repo Repository, remoteName string, timeout time.Duration, stdout, stderr io.Writer) error {
+	return git.NewCommand("remote", "update", "--prune").AddDynamicArguments(remoteName).


Here and above, why git remote update/prune belongs to "config.go"?

wxiaoguang · 2025-08-19T19:09:50Z

modules/gitrepo/config.go

+	releaser, err := globallock.Lock(ctx, getRepoConfigLockKey(repo.RelativePath()))
+	if err != nil {
+		return err
+	}
+	defer releaser()


All these "lock" related code could be in a helper wrapper function:

gitConfigLock(..., func() { ... })

wxiaoguang · 2025-08-19T19:10:55Z

modules/gitrepo/config.go

+	if addr == "" {
+		return nil, nil
+	}


I believe this design is problematic.

Developers have been used to that "if err == nil, then there is a non-nil return value"

If you do so, developers would forget to check "u == nil" frequently, and the nil check introduces unnecessary code.

wxiaoguang · 2025-08-19T19:11:42Z

services/mirror/mirror_pull.go

 	if err != nil && !git.IsRemoteNotExistError(err) {
 		return err
 	}

-	cmd := git.NewCommand("remote", "add").AddDynamicArguments(remoteName).AddArguments("--mirror=fetch").AddDynamicArguments(addr)
-	_, _, err = cmd.RunStdString(ctx, &git.RunOpts{Dir: repoPath})
+	err = gitrepo.GitRemoteAdd(ctx, repo, remoteName, addr, "--mirror=fetch")


Here and other places, IIRC you have defined consts, but why still use string.

wxiaoguang · 2025-08-19T19:17:43Z

services/mirror/mirror_pull.go

-				Stderr:  &stderrBuilder,
-			}); err != nil {
+
+		// check whether the remote still exists before pruning to avoid prune creating a new remote


to avoid prune creating a new remote

What "prune"? "git remote prune" or "git remote update --prune"?

I see neither of them would "create a new remote" in the git manual. Correct me if I was wrong.

…a into lunny/fix_git_config_conflict

wxiaoguang · 2025-08-20T03:53:09Z

services/mirror/mirror_pull.go

+		// check whether the remote still exists before remote update prune
+		// this is needed because prune will not fail if the remote does not exist


What "prune"? "git remote prune" or "git remote update --prune"?

below does git remote update --prune

pruneBrokenReferences does git remote prune

So what are you doing here for?

I see NEITHER of them would "create a new remote" in the git manual. Correct me if I was wrong.

Why "this is needed because prune will not fail if the remote does not exist" is a problem?

Why you need this logic?

It said before remote update prune ..

It said before remote update prune ..

Why it needs this logic? What wrong would happen if remote doesn't exist?

I updated the comment via 38c19d2

Why it needs this logic? What wrong would happen if remote doesn't exist?

Why the "remote" might not exist?

What's wrong with "it will appear to succeed silently, even though nothing was updated."?

If you return "false" here, what end users can see or can do?

I remove these lines via a9633fd

Move git config/remote to gitrepo package and add global lock to reso…

a38ff9a

…lve possible conflict when updating repository git config file

lunny added the type/bug label Jul 24, 2025

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 24, 2025

github-actions bot added modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code labels Jul 24, 2025

remove unnecessary change

2e69ad3

wxiaoguang previously requested changes Jul 24, 2025

View reviewed changes

GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jul 24, 2025

Fix bug

2f89a04

GiteaBot added lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. and removed lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged labels Jul 25, 2025

fix

183cde5

wxiaoguang reviewed Jul 25, 2025

View reviewed changes

modules/gitrepo/config.go Outdated Show resolved Hide resolved

fix

70ef126

wxiaoguang reviewed Jul 25, 2025

View reviewed changes

modules/gitrepo/config.go Outdated Show resolved Hide resolved

wxiaoguang marked this pull request as draft July 25, 2025 03:15

lunny added 4 commits July 25, 2025 11:07

some improvements

98bc0df

Merge branch 'lunny/fix_git_config_conflict' of github.com:lunny/gite…

976df17

…a into lunny/fix_git_config_conflict

improvements

7a8fdc2

improvements

869f3ae

lunny marked this pull request as ready for review July 26, 2025 18:06

lunny requested a review from wxiaoguang July 26, 2025 18:06

hiifong approved these changes Jul 31, 2025

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jul 31, 2025

lunny added this to the 1.25.0 milestone Jul 31, 2025

Merge branch 'main' into lunny/fix_git_config_conflict

6501cff

lunny mentioned this pull request Aug 15, 2025

GetCompareInfo, AddRemote: exit status 128 - error: could not lock config file config: File exists #32018

Open

wxiaoguang reviewed Aug 19, 2025

View reviewed changes

lunny added 2 commits August 19, 2025 16:43

Follow @wxiaoguang's suggestion

ae3c338

Merge branch 'lunny/fix_git_config_conflict' of github.com:lunny/gite…

00666bd

…a into lunny/fix_git_config_conflict

wxiaoguang reviewed Aug 20, 2025

View reviewed changes

lunny added 2 commits August 19, 2025 21:00

update comment

38c19d2

follow wxiaoguang's suggestion

a9633fd

		// check whether the remote still exists before remote update prune
		// this is needed because prune will not fail if the remote does not exist

Uh oh!

Move git config/remote to gitrepo package and add global lock to resolve possible conflict when updating repository git config file #35151

Are you sure you want to change the base?

Move git config/remote to gitrepo package and add global lock to resolve possible conflict when updating repository git config file #35151

Conversation

lunny commented Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wxiaoguang commented Jul 24, 2025

Uh oh!

wxiaoguang left a comment

Choose a reason for hiding this comment

Uh oh!

lunny commented Jul 24, 2025

Uh oh!

wxiaoguang commented Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lunny commented Jul 25, 2025

Uh oh!

wxiaoguang commented Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lunny commented Aug 19, 2025

Uh oh!

wxiaoguang Aug 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wxiaoguang Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lunny commented Jul 24, 2025 •

edited

Loading

wxiaoguang commented Jul 25, 2025 •

edited

Loading

wxiaoguang commented Jul 25, 2025 •

edited

Loading

wxiaoguang Aug 19, 2025 •

edited

Loading

wxiaoguang Aug 20, 2025 •

edited

Loading